fix: preserve ProjectionExec metadata during serialization - #25009
fix: preserve ProjectionExec metadata during serialization#25009goutamadwant wants to merge 2 commits into
Conversation
github-actions
Bot
added
proto
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25009 +/- ##
==========================================
+ Coverage 81.90% 81.95% +0.04%
==========================================
Files 1134 1134
Lines 425261 425293 +32
Branches 425261 425293 +32
==========================================
+ Hits 348325 348559 +234
+ Misses 56295 56049 -246
- Partials 20641 20685 +44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @goutamadwant !
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
One concern, other than that thank you 👍
| let input = ctx.encode_child(input)?; | ||
| let expr = ctx.encode_expressions(projection_exprs.iter().map(|p| &p.expr))?; | ||
| let expr_name = projection_exprs.iter().map(|p| p.alias.clone()).collect(); | ||
| let schema = if *overrides_metadata { |
There was a problem hiding this comment.
This conditioins shouldnt just be if we are overriding metadata I believe. Rather we should be checking if we have metadata in general.
There was a problem hiding this comment.
@gene-bordegaray Updated the condition to encode schema or output-field metadata even when inherited. The override check remains so an empty metadata map can still explicitly clear input metadata.
| )); | ||
| let plan = Arc::new(ProjectionExec::try_new( | ||
| vec![(col("value", &input_schema)?, "value".to_string())], | ||
| Arc::new(EmptyExec::new(Arc::clone(&input_schema))), |
There was a problem hiding this comment.
this is preserving the metadata through the child after decoding, not a proojection that needs to rederive the metadata completely from itself.
Could we add a test that forces the projection to completely rederive the metadata from its own proto after roundtrip 👍
There was a problem hiding this comment.
@gene-bordegaray added a regression that removes metadata from the encoded child and checks the decoded projection independently. Let me know if it looks good now.
4 participants
Which issue does this PR close?
Rationale for this change
Physical-plan serialization can lose projection field and schema metadata. The projection's payload should preserve that metadata without relying on the child to reconstruct it.
What changes are included in this PR?
ProjectionExecNode, emitted when the projection has schema or output-field metadata, or explicitly clears input metadata.try_new_with_schema_metadata, keeping field names, data types, and nullability derived from the expressions.What is the testing strategy for this PR?
./dev/rust_lint.shchecks pass, including the private-item documentation build.Are there any user-facing changes?
Projection schema and output-field metadata survive physical-plan serialization. Nested field definitions remain part of the expression-derived data types. The generated
ProjectionExecNodeRust struct gains an optionalschemafield, which affects exhaustive struct literals. New readers continue to accept older payloads. This targets main, not a 55.1 backport.